bpo-18283: Add support for bytes to shutil.which#11818
Conversation
| # Check that a given file can be accessed with the correct mode. | ||
| # Additionally check that `file` is not a directory, as on Windows | ||
| # directories pass the os.access check. | ||
| def _access_check(fn, mode): |
There was a problem hiding this comment.
Just curious any reason over moving this outside which?
There was a problem hiding this comment.
As an author of the original patch, I can explain: it's inefficient to define a temporary function for each which() call. Moreover, there is no technical reason to use a nested function.
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
LGTM... well, I wrote the initial patch, so I'm not fair :-)
Just a few suggestions.
| # PATHEXT is necessary to check on Windows. | ||
| pathext = os.environ.get("PATHEXT", "").split(os.pathsep) | ||
| if isinstance(cmd, bytes): | ||
| pathext = list(map(os.fsencode, pathext)) |
There was a problem hiding this comment.
Maybe use a list-comprehension instead? I'm not sure what is the best ;-)
There was a problem hiding this comment.
I think that list(map(os.fsencode, pathext)) it's ok.
There was a problem hiding this comment.
I did a timeit on both and the list(map()) is slightly faster (on my system), but not much. If it code after this didn't (potentially) iterate over pathext twice, a generator would be ideal.
Also, the following if clause uses:
if any(cmd.lower().endswith(ext.lower()) for ext in pathext):
files = [cmd]
Removing the cmd.lower() from the any would be a processing optimization, but it's unrelated to this change, so I didn't put it in.
cmd_lower = cmd.lower()
if any(cmd_lower.endswith(ext.lower()) for ext in pathext):
files = [cmd]
eamanu
left a comment
eamanu
left a comment
There was a problem hiding this comment.
LGTM. it's work correctly
| # PATHEXT is necessary to check on Windows. | ||
| pathext = os.environ.get("PATHEXT", "").split(os.pathsep) | ||
| if isinstance(cmd, bytes): | ||
| pathext = list(map(os.fsencode, pathext)) |
There was a problem hiding this comment.
I think that list(map(os.fsencode, pathext)) it's ok.
|
@giampaolo: I wrote the initial patch, so I would prefer that someone else would review it. Would mind to review this change please? |
|
@vstinner no problem; patch LGTM. |
|
Thanks @giampaolo for the review ;-) |
|
Thanks @csabella, I merged your PR ;-) |
7 participants
Original patch by Victor Stinner.
https://bugs.python.org/issue18283